Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return 'self' in some widget verb methods. #2102

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

rodrigogiraoserrao
Copy link
Contributor

I compiled a list of all widget methods that return None and for which it could make sense to make this change. (I filtered out some methods, like watch and action methods.)

I tried choosing a subset of those methods, trying to only pick methods for which there weren't two things that could be returned (e.g., Widget.move_child could return either the widget or the child that was moved) and I also tried to only pick methods that have little or no parameters (e.g., Widget.animate has many parameters and is typically called with quite a few.)

These are all the Widget methods for which this could make sense:

  • move_child (either return self or the actual child that was moved…)
  • animate
  • scroll_to / scroll_relate / scroll_home / scroll_end / scroll_left / scroll_right / scroll_down / scroll_up / scroll_page_up / scroll_page_down / scroll_page_left / scroll_page_right / scroll_visible
  • refresh
  • focus / reset_focus
  • capture_mouse / release_mouse

Additionally, I looked at each widget, and found these methods:

  • Tree
    • TreeNode
      • expand / expand_all / collapse / collapse_all / toggle / toggle_all
      • set_label
    • clear / reset
    • select_node (either return self or the actual node that was selected)
    • scroll_to_line / scroll_to_node
    • refresh_line
  • ToggleButton
    • toggle (and action_toggle?)
  • TextLog
    • write
    • clear
  • Tabs
    • add_tab / remove_tab
    • clear
  • Switch
    • toggle (and action_toggle?)
  • Static
    • update
  • Pretty
    • update
  • Placeholder
    • cycle_variant
  • _markdown.py
    • MarkdownBlock
      • set_content
    • MarkdownTableOfContents
      • set_table_of_contents
  • Input
    • insert_text_at_cursor
  • DirectoryTree
    • load_directory
  • DataTable
    • update_cell / update_cell_at
    • clear
    • refresh_coordinate / refresh_row / refresh_column
    • sort
  • Button
    • press

Related issues: #1908
Related discussions: #1817

I compiled a list of all widget methods that return 'None' and for which it _could_ make sense to make this change.
(I filtered out some methods, like watch and action methods.)

I tried choosing a subset of those methods, trying to only pick methods for which there weren't two things that could be returned (e.g., 'Widget.move_child' _could_ return either the widget or the child that was moved) and I also tried to only pick methods that have little or no parameters (e.g., 'Widget.animate' has many parameters and is typically called with quite a few.

These are all the 'Widget' methods for which this could make sense:
- 'move_child' (either return 'self' or the actual 'child' that was moved…)
- 'animate'
- 'scroll_to' / 'scroll_relate' / 'scroll_home' / 'scroll_end' / 'scroll_left' / 'scroll_right' / 'scroll_down' / 'scroll_up' / 'scroll_page_up' / 'scroll_page_down' / 'scroll_page_left' / 'scroll_page_right' / 'scroll_visible'
- 'refresh'
- 'focus' / 'reset_focus'
- 'capture_mouse' / 'release_mouse'

Additionally, I looked at each widget, and found these methods:
- 'Tree'
    - 'TreeNode'
        - 'expand' / 'expand_all' / 'collapse' / 'collapse_all' / 'toggle' / 'toggle_all'
        - 'set_label'
    - 'clear' / 'reset'
    - 'select_node' (either return 'self' or the actual 'node' that was selected)
    - 'scroll_to_line' / 'scroll_to_node'
    - 'refresh_line'
- 'ToggleButton'
    - 'toggle' (and 'action_toggle'?)
- 'TextLog'
    - 'write'
    - 'clear'
- 'Tabs'
    - 'add_tab' / 'remove_tab'
    - 'clear'
- 'Switch'
    - 'toggle' (and 'action_toggle'?)
- 'Static'
    - 'update'
- 'Pretty'
    - 'update'
- 'Placeholder'
    - 'cycle_variant'
- '_markdown.py'
    - 'MarkdownBlock'
        - 'set_content'
    - 'MarkdownTableOfContents'
        - 'set_table_of_contents'
- 'Input'
    - 'insert_text_at_cursor'
- 'DirectoryTree'
    - 'load_directory'
- 'DataTable'
    - 'update_cell' / 'update_cell_at'
    - 'clear'
    - 'refresh_coordinate' / 'refresh_row' / 'refresh_column'
    - 'sort'
- 'Button'
    - 'press'

Related issues: #1908
Related discussions: #1817
src/textual/widgets/_tree.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. But I think it would be best to always explicitly return self, rather than return the result of another method.

src/textual/widgets/_data_table.py Outdated Show resolved Hide resolved
src/textual/widgets/_data_table.py Outdated Show resolved Hide resolved
@rodrigogiraoserrao
Copy link
Contributor Author

[...] always explicitly return self [...]

That makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants